-
Notifications
You must be signed in to change notification settings - Fork 359
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: Improve CompareTrials perfomance #9807
Conversation
✅ Deploy Preview for determined-ui canceled.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9807 +/- ##
==========================================
+ Coverage 54.26% 54.28% +0.01%
==========================================
Files 1261 1261
Lines 155671 155782 +111
Branches 3536 3534 -2
==========================================
+ Hits 84481 84560 +79
- Misses 71052 71084 +32
Partials 138 138
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, missing some test coverage
@@ -521,6 +521,24 @@ WHERE t.id = ?`, trialID).Scan(ctx, &experiment); err != nil { | |||
return &experiment, nil | |||
} | |||
|
|||
// ExperimentsByTrialID looks up an experiment by a given list of trialIDs, returning | |||
// an error if none exists. | |||
func ExperimentsByTrialID(ctx context.Context, trialIDs []int) ([]*model.Experiment, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blocker -- missing a test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test added.
}) | ||
return err | ||
}, false}, | ||
// {"CanGetExperimentArtifacts", func(id int) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this an intended change? if not, please uncomment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry there was an error with this that I forgot to fix. Done now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding this to the performance tests - the results look good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Ticket
ET-685
Description
Improving performance of run comparison.
Test Plan
Select a large amount of runs (>50) on the Run Table and click compare. Ensure that the time it takes to load is resonalble
Checklist
docs/release-notes/
See Release Note for details.